databusclient/cli.py (2)

14-56: Consider several improvements to the parsing logic.

1. Warning mechanism (Line 49): Using print() for warnings writes directly to stdout, which is not ideal for library code. Consider using Python's logging module or collecting warnings in the return value for better testability and control.

2. Compression heuristic is fragile (Lines 36-39): The classification of .tar as compression is questionable since .tar is often treated as an archive format, not a compression format. Files like .tar.gz have both archiving and compression. This heuristic may misclassify extensions and lead to incorrect metadata.

3. Missing docstring examples: The docstring should include concrete examples showing input and expected output to clarify the expected format and behavior.

4. No validation of key=value pairs (Line 43): The code splits on '=' but doesn't validate that the key or value is non-empty. Consider adding validation to reject malformed variants like "=value" or "key=".

5. Inconsistent extension handling: Line 37's comment suggests conditional removal of the leading dot ("if needed"), but the code always strips it. Clarify the expected format or document why the leading dot is always removed.

🔎 Suggested improvements

Replace print() with proper logging:

+import logging
+
+logger = logging.getLogger(__name__)
+
 def parse_distribution_str(dist_str: str):
     """
     Parses a distribution string with format:
     URL|key=value|...|.extension
     
     Returns a dictionary suitable for the deploy API.
+    
+    Examples:
+        >>> parse_distribution_str("http://example.com/data.json|lang=en|.json|.gz")
+        {'url': 'http://example.com/data.json', 'variants': {'lang': 'en'}, 
+         'formatExtension': 'json', 'compression': 'gz'}
     """
     ...
         else:
-             print(f"WARNING: Unrecognized modifier '{part}' in distribution. Expected '.ext' or 'key=val'.")
+             logger.warning(f"Unrecognized modifier '{part}' in distribution. Expected '.ext' or 'key=val'.")

Add validation for key=value pairs:

         elif '=' in part:
             key, value = part.split('=', 1)
+            if not key.strip() or not value.strip():
+                logger.warning(f"Invalid key=value pair '{part}' in distribution.")
+                continue
             variants[key.strip()] = value.strip()

---

129-137: Remove developer scaffolding comments and consider error handling.

1. Lines 129, 137: The "--- CHANGE START/END ---" comments are developer scaffolding and should be removed before merging.

2. Line 131: Consider adding error handling around parse_distribution_str calls. If parsing fails (e.g., malformed distribution string), the user should receive a clear error message rather than an uncaught exception.

3. Behavioral change: This modification changes the contract between the CLI and api_deploy.create_dataset. Previously, raw strings were passed; now parsed dictionaries are passed. While this appears to be intentional and supported by changes in deploy.py, it's worth verifying that all downstream consumers can handle this change.

🔎 Proposed cleanup

-        # --- CHANGE START ---
-        # Parse the input strings into structured objects
         parsed_distributions = [parse_distribution_str(d) for d in distributions]
         
-        # Note: api_deploy.create_dataset now accepts this list of dicts
         dataid = api_deploy.create_dataset(
             version_id, title, abstract, description, license_url, parsed_distributions
         )
-        # --- CHANGE END ---

databusclient/api/queries.py (1)

54-93: Consider adding validation for malformed key=value pairs.

Similar to parse_distribution_str in cli.py, this function splits on '=' (Line 87) but doesn't validate that both key and value are non-empty. Consider adding a check to handle edge cases like "=value" or "key=".

🔎 Suggested validation

         if "=" in part:
             key, value = part.split("=", 1)
+            if not key.strip() or not value.strip():
+                # Skip malformed key=value pairs
+                continue
             variants[key.strip()] = value.strip()

tests/test_parse_distribution.py (1)

1-275: Excellent test coverage with minor gaps.

The test suite is comprehensive and well-structured:

• Clear test organization with descriptive test names
• Good coverage of parsing logic: URL extraction, variants, extensions, compression
• Appropriate use of mocking to avoid network dependencies
• Integration tests validate end-to-end behavior with the deploy API

Minor suggestions for additional test coverage:

1. Error handling: Consider adding tests for truly malformed inputs that might cause exceptions (e.g., None input, non-string input, extremely long strings).

2. Edge case - URLs with delimiter: Test behavior when the URL itself contains the '|' delimiter character (e.g., "http://example.com/file?param=a|b|.json"). The current implementation would split incorrectly.

3. Empty parts: Test distribution strings with empty parts between delimiters (e.g., "http://example.com/file||.json" with double pipe).

🔎 Example additional tests

def test_url_with_pipe_character(self):
    """Test URL containing pipe character in query string."""
    # This is an edge case that might need special handling
    result = parse_distribution_str("http://example.com/file?param=a|b")
    # Current implementation would incorrectly split on the pipe in the URL
    assert result["url"] == "http://example.com/file?param=a|b"

def test_empty_parts_between_delimiters(self):
    """Test handling of empty parts between delimiters."""
    result = parse_distribution_str("http://example.com/file||.json")
    # Should gracefully handle empty parts
    assert result["formatExtension"] == "json"

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro